Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: exclude text processors from Stream.getProcessors when text is disabled #3342

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

03k64
Copy link

@03k64 03k64 commented Jul 21, 2020

This fixes a bug observed with low-latency streams containing one or more text representations. The bug prevented the liveCatchUpPlaybackRate from being correctly applied following a buffer stall, leading to unconstrained drift from the user-specified target latency.

The bug manifested as follows:

  1. a buffer stall event occurs, the playbackStalled field on StreamController is set to true
  2. StreamController.startPlaybackCatchUp is called, it uses StreamController.getBufferLevel to obtain the lowest current audio, video text or fragmented text buffer level
  3. in streams with a text or fragmented text representation, the recent change to disable text by default resulted in an empty buffer being maintained for that representation unless text had been explicitly enabled for the player
  4. the empty buffer prevents StreamController.startPlaybackCatchUp from resetting the playbackStalled field of StreamController to false, further, it assumes a buffer stall is ongoing, and resets the newRate to 1.0, preventing latency from being reduced

This change then checks whether text has been enabled and only adds the stream processors for textual representations to those returned by the Stream.getProcessors if it is. I have added some tests to document/verify the new intended behaviour. I have also included a smaller change, that modifies a function-level variable name to provide clarity due to a class-level variable with the same name.

This is my first PR, however I am submitting as part of my work at the BBC, who have previously signed up to the dash.js feedback agreement.

- removes potential for ambiguity between liveDelay variable in
  startPlaybackCatchUp function and PlaybackController module when
  reasoning about the code
- new name intended to convey that the player's liveDelay value is used
  throughout this function
This fixes a bug observed with low-latency streams containing one or
more text representations. The bug prevented the liveCatchUpPlaybackRate
from being correctly applied following a buffer stall, leading to
unconstrained drift from the user-specified target latency.

The bug manifested as follows:

1. a buffer stall event occurs, the 'playbackStalled' field on
   StreamController is set to 'true'
2. 'StreamController.startPlaybackCatchUp' is called, it uses
   'StreamController.getBufferLevel' to obtain the lowest current audio,
   video text or fragmented text buffer level
3. in streams with a text or fragmented text representation, the recent
   change to disable text by default resulted in an empty buffer being
   maintained for that representation _unless_ text had been explicitly
   enabled for the player
4. the empty buffer prevents 'StreamController.startPlaybackCatchUp'
   from resetting the 'playbackStalled' field of 'StreamController' to
   'false', further, it assumes a buffer stall is ongoing, and resets
   the 'newRate' to '1.0', preventing latency from being reduced

This change then checks whether text has been enabled and only adds the
stream processors for textual representations to those returned by the
'Stream.getProcessors' if it is.

Tested on Firefox 78.02 & Chrome 84.0.414.79 on Ubuntu 20.04.
- existing test verifies that empty array of stream processors exists
when stream is not activated
- additional tests check that text and fragmented text processors are
returned by Stream.getProcessors _only_ when text is enabled via the
TextController
This fix ensure the correct minimum buffer level is reported when text
is disabled. Previously a buffer level of 0 is reported at all times if
a text track is present in the manifest but text is disabled in the
player. This fix checks whether text is enabled and only includes text
buffer metrics in buffer level reporting if it is.
@dsilhavy dsilhavy added this to the 3.2.0 milestone Oct 12, 2020
@dsilhavy dsilhavy self-requested a review October 12, 2020 15:37
@dsilhavy dsilhavy modified the milestones: 3.2.0, 3.2.1 Dec 4, 2020
@dsilhavy dsilhavy modified the milestones: 3.2.1, 3.3.0 Feb 15, 2021
@@ -714,6 +714,8 @@ function Stream(config) {
}

function getProcessors() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getProcessors is used in other functions as well. I would prefer to add a flag to this function "includeTextIfDisabled" or something similar. This flag should default to true and should only be false for the use case you describe.

@dsilhavy
Copy link
Collaborator

dsilhavy commented Apr 1, 2021

@03k64 Sorry for the delay in reviewing this. I had one comment, see above. Can you also rebase against the current development branch please? Then we can add this to 3.2.2. Thank you!

@dsilhavy dsilhavy modified the milestones: 3.2.2, 4.0.0 Apr 5, 2021
@dsilhavy
Copy link
Collaborator

@03k64 Can you give me short feedback if you are still interested in contributing this? Otherwise I might pick this up and rebase it against the current dev branch.

@03k64
Copy link
Author

03k64 commented May 28, 2021

@03k64 Can you give me short feedback if you are still interested in contributing this? Otherwise I might pick this up and rebase it against the current dev branch.

Hi, sorry, things have been kind of hectic for the last while - I made the original PR while working on an internship last summer at the BBC. I won't be able to commit the time for the next few weeks (probably not until the second half of June) so if you'd like to work on it before then then please do :) otherwise I will be happy to make the changes you suggested at that point 👍

@dsilhavy dsilhavy modified the milestones: 4.0.0, 4.0.1 Jun 9, 2021
@dsilhavy dsilhavy modified the milestones: 4.0.1, 4.1.0 Jun 27, 2021
@dsilhavy dsilhavy modified the milestones: 4.1.0, 4.1.1 Sep 15, 2021
@dsilhavy dsilhavy removed this from the 4.2.0 milestone Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants